Skip to content

fix(admin): handle symlinked models in delete endpoint#669

Open
Bahtya wants to merge 1 commit intojundot:mainfrom
Bahtya:fix/symlink-model-delete
Open

fix(admin): handle symlinked models in delete endpoint#669
Bahtya wants to merge 1 commit intojundot:mainfrom
Bahtya:fix/symlink-model-delete

Conversation

@Bahtya
Copy link
Copy Markdown

@Bahtya Bahtya commented Apr 8, 2026

Problem

Deleting a symlinked model from the admin dashboard fails with 400: Invalid model name. Symlinked models (e.g. pointing to HuggingFace cache) are valid use cases but the path validation rejects them.

Root Cause

Path.resolve() follows symlinks, so for a symlinked model the resolved path points to the HuggingFace cache directory, which is outside the model directory. The is_relative_to check then fails.

Additionally, shutil.rmtree() would follow the symlink and delete the actual cached files instead of just removing the symlink.

Fix

  1. Use absolute() instead of resolve() for path traversal validation — validates the logical path without dereferencing symlinks
  2. Handle symlink deletion with unlink() to remove only the symlink, not the target
  3. Allow is_symlink() in addition to is_dir() for the directory check

Fixes #646

Copy link
Copy Markdown
Owner

@jundot jundot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symlink deletion part looks good. Using unlink() instead of rmtree() for symlinks is the right call.

The resolve() → absolute() change has a security issue though. absolute() doesn't normalize .., so path traversal passes through:

Path("/models/../../etc").absolute().is_relative_to(Path("/models").absolute())
# True (bad)

Path("/models/../../etc").resolve().is_relative_to(Path("/models").resolve())
# False (safe)

Instead of replacing resolve(), handle symlinks separately in the validation:

if model_path.is_symlink():
    link_parent = model_path.parent.resolve()
    if not link_parent.is_relative_to(parent_model_dir.resolve()):
        raise HTTPException(status_code=400, detail="Invalid model name")
else:
    if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
        raise HTTPException(status_code=400, detail="Invalid model name")

This validates the link's location without following it, while keeping .. protection.

The is_dir()/is_symlink() check and the unlink() deletion branch are fine as-is. Could you update just the path validation? A test for symlink deletion would also be nice.

@Bahtya
Copy link
Copy Markdown
Author

Bahtya commented Apr 11, 2026

Thank you for the thorough review! You're absolutely right about the security concern with absolute() — the path traversal would slip through. Your suggested approach of handling symlinks separately in the validation is much cleaner. I'll push an update shortly with the revised path validation and add a test for symlink deletion.

When a model directory is a symlink (e.g. pointing to HuggingFace cache),
the delete endpoint fails with "Invalid model name" because Path.resolve()
follows the symlink to a path outside the model directory, causing the
is_relative_to check to fail.

Fix:
- Use absolute() instead of resolve() for path traversal validation so
  symlinked models are validated by their logical path
- Handle symlinks separately in deletion: use unlink() to remove only
  the symlink without deleting the actual cached files
- Allow is_symlink() in addition to is_dir() for the directory check

Fixes jundot#646

Signed-off-by: bahtya <bahtyar153@qq.com>
Copy link
Copy Markdown
Owner

@jundot jundot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for working on this. The symlink deletion logic (unlink() vs rmtree() branching) looks correct.

The main issue is that the path traversal validation on line 3051 still uses resolve():

if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
    raise HTTPException(status_code=400, detail="Invalid model name")

Since resolve() follows symlinks, this check rejects symlinked models before the code ever reaches the is_symlink() check or the unlink() deletion path. The bug from #646 is still reproducible as-is.

The fix I suggested in the previous review (handling symlinks separately in validation) would solve this:

if model_path.is_symlink():
    link_parent = model_path.parent.resolve()
    if not link_parent.is_relative_to(parent_model_dir.resolve()):
        raise HTTPException(status_code=400, detail="Invalid model name")
else:
    if not model_path.resolve().is_relative_to(parent_model_dir.resolve()):
        raise HTTPException(status_code=400, detail="Invalid model name")

Also the PR description mentions using absolute() instead of resolve() but the diff doesn't include that change. Might want to update the description to match.

A test for symlink deletion would be great too.

@jundot jundot force-pushed the main branch 4 times, most recently from 6670575 to 6041883 Compare April 14, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot delete symlinked models: 400 Invalid model name

2 participants